Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass analyze #819

Merged
merged 5 commits into from Nov 5, 2015
Merged

Pass analyze #819

merged 5 commits into from Nov 5, 2015

Conversation

richafrank
Copy link
Member

@elmq0022 , Since #818 was facing conflicts and such, I rebased and fixed up your branch. If this looks right to you, and tests pass, we can merge this one in.

puppy and others added 5 commits November 5, 2015 10:16
_analyze Method in algorithm.py

Added one line to check for the keyword argument 'analyze' and set the
the _analyze method when a TradingAlgorithm object is initialized
within a script.
@elmq0022
Copy link

elmq0022 commented Nov 5, 2015

It looks great and all the test passed.

I assume the merge has to be done on your side, correct? Do you want me to
go back and close the issue #801 and pull request #818?

Also, this is my first contribution to an open source project. Honestly it
has been more of a learning curve than I expected. Thank you for helping
me get over the hump.

On Thu, Nov 5, 2015 at 9:23 AM, Richard Frank notifications@github.com
wrote:

@elmq0022 https://github.com/elmq0022 , Since #818
#818 was facing conflicts and
such, I rebased and fixed up your branch. If this looks right to you, and

tests pass, we can merge this one in.

You can view, comment on, or merge this pull request online at:

#819
Commit Summary

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#819.

@richafrank
Copy link
Member Author

It looks great and all the test passed.

Great!

I assume the merge has to be done on your side, correct? Do you want me to go back and close the issue #801 and pull request #818?

That's correct. I can update everything on merging.

Also, this is my first contribution to an open source project. Honestly it has been more of a learning curve than I expected. Thank you for helping me get over the hump.

And we really appreciate it! Sorry there's such a curve, but I'm happy to help! And we're always looking for ways to improve the experience...

@elmq0022
Copy link

elmq0022 commented Nov 5, 2015

Nah, wasn't that bad. The just more issues than I was expecting, mostly
related to my environment. There seems to be some dependencies beyond what
the list in the development guide: SQLAlchemy, parameterized-nose, toolz,
and cython to name a few.

If I can get these all resolved and actually run the test suite and build
the docs, things will go better. In fact, getting this done and updating
the developer guide may be a good next task.

On Thu, Nov 5, 2015 at 10:30 AM, Richard Frank notifications@github.com
wrote:

It looks great and all the test passed.

Great!

I assume the merge has to be done on your side, correct? Do you want me to
go back and close the issue #801
#801 and pull request #818
#818?

That's correct. I can update everything on merging.

Also, this is my first contribution to an open source project. Honestly it
has been more of a learning curve than I expected. Thank you for helping me
get over the hump.

And we really appreciate it! Sorry there's such a curve, but I'm happy to
help! And we're always looking for ways to improve the experience...


Reply to this email directly or view it on GitHub
#819 (comment).

@richafrank
Copy link
Member Author

Oh interesting! I think those dependencies should be in requirements.txt or requirements_dev.txt, but definitely let us know if anything is missing or submit an update the guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants